Skip to content

Fix bugs with SCIP field enclosing_range #21271

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
CatherineGasnier wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix bugs with SCIP field enclosing_range #21271

CatherineGasnier wants to merge 1 commit into from
+348 −18

Conversation

@CatherineGasnier
Copy link

@CatherineGasnier CatherineGasnier commented Dec 15, 2025 •
edited by rustbot
Loading

Summary: Previously, the enclosing range of a function name would be the range of the whole enclosing impl. I'm not sure exactly why, but the old code for enclosing_range was just not doing the right thing. The new doc comments should make it clear what we're trying to achieve in this new version.

Related PR: #21141

Test Plan:
Added unit test.
Also tested on Graphite repo

@CatherineGasnier CatherineGasnier force-pushed the fix-scip-enclosing_range branch 2 times, most recently from b17b256 to 7a1b1a0 Compare December 16, 2025 13:58
@CatherineGasnier CatherineGasnier marked this pull request as ready for review December 16, 2025 14:04
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2025
ChayimFriedman2
ChayimFriedman2 requested changes Dec 21, 2025
View reviewed changes
Copy link
Contributor

@ChayimFriedman2 ChayimFriedman2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code looks correct to me, the changes here aren't. The culprit is likely isn't here.

@CatherineGasnier CatherineGasnier force-pushed the fix-scip-enclosing_range branch 2 times, most recently from d000853 to b0eaa1f Compare January 14, 2026 17:16
@CatherineGasnier
Copy link
Author

CatherineGasnier commented Jan 14, 2026

ok, after further examination, I think the culprit was likely in get_definitions which has a fixme and probably does something weird with macros (the code I was testing on had macros). I'm going for a different approach in this new version, which avoids using the result of get_definitions, and also allows to compute the enclosing_range for all occurrences, not just occurrences that are definitions. I've also added a ton of tests. The previous implementation would fail more than half of those tests.

Fix bugs with SCIP field enclosing_range
8e3d360 
Summary:

Test Plan:

Reviewers:

Subscribers:

Tasks:

Tags:
@CatherineGasnier CatherineGasnier force-pushed the fix-scip-enclosing_range branch from b0eaa1f to 8e3d360 Compare January 14, 2026 19:01
@CatherineGasnier
Copy link
Author

CatherineGasnier commented Jan 14, 2026

@ChayimFriedman2 I've just updated a new version fixing the CI. Can you detail what changes you request? Do you still think the code is incorrect? If so why, given all the new tests I added pass? Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChayimFriedman2 ChayimFriedman2 Awaiting requested review from ChayimFriedman2

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@CatherineGasnier @ChayimFriedman2 @rustbot